Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop storing stateless kubernetes keystores #21880

Merged
merged 8 commits into from
Oct 19, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Oct 16, 2020

What does this PR do?

This PR removes kubernetesKeystores since it does not store anything that actually needs to be shared.

Related issues

@ChrsMark ChrsMark self-assigned this Oct 16, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 16, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 16, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #21880 updated]

  • Start Time: 2020-10-19T13:31:17.436+0000

  • Duration: 90 min 52 sec

Test stats 🧪

Test Results
Failed 0
Passed 16368
Skipped 1343
Total 17711

@ChrsMark ChrsMark changed the title Protect keystore map with mutex Protect kubernetes keystore map with mutex Oct 16, 2020
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Comment on lines 82 to 83
storedKeystore, _ := kr.kubernetesKeystores.LoadOrStore(namespace, k8sKeystore)
return storedKeystore.(keystore.Keystore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we are instantiating a new keystore on every call, this is why I mentioned that we could need to store an object that creates the keystore lazily.

But, I have been looking at the implementation of the k8s keystore and it actually doesn't keep any state at all, so maybe we can return a new one each time and don't keep a registry?

Suggested change
storedKeystore, _ := kr.kubernetesKeystores.LoadOrStore(namespace, k8sKeystore)
return storedKeystore.(keystore.Keystore)
return k8sKeystore

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark changed the title Protect kubernetes keystore map with mutex Stop storing stateless kubernetes keystores Oct 19, 2020
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only a suggestion about a comment.

@@ -76,11 +71,7 @@ func (kr *KubernetesKeystoresRegistry) GetKeystore(event bus.Event) keystore.Key
}
if namespace != "" {
// either retrieve already stored keystore or create a new one for the namespace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not needed anymore.

Suggested change
// either retrieve already stored keystore or create a new one for the namespace

Signed-off-by: chrismark <chrismarkou92@gmail.com>
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
@ChrsMark ChrsMark merged commit b2d1929 into elastic:master Oct 19, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Oct 19, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Oct 19, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Oct 19, 2020
ChrsMark added a commit that referenced this pull request Oct 20, 2020
ChrsMark added a commit that referenced this pull request Oct 20, 2020
v1v added a commit to v1v/beats that referenced this pull request Oct 21, 2020
* upstream/master:
  feat: package aliases for snapshots (elastic#21960)
  [DOC] Add firewall as possible troubleshooting issue (elastic#21743)
  [Filebeat] Add max_number_of_messages config parameter for S3 input (elastic#21993)
  [Elastic Agent] Fix missing elastic_agent event data  (elastic#21994)
  Document auditbeat system process module config (elastic#21766)
  Update links (elastic#22012)
  dynamically find librpm (elastic#21936)
  Fix Istio docs (elastic#22019)
  [beats-tester][packaging] store packages in another location (elastic#21903)
  [Kubernetes] Remove redundant dockersock volume mount (elastic#22009)
  [Ingest Manager] Always try snapshot repo for agent upgrade (elastic#21951)
  Azure storage metricset values not inside the metricset name (elastic#21845)
  fix diskio and memory bugs under windows (elastic#21992)
  Fix TestDockerStart flaky test (elastic#21681)
  filebeat: add SSL options to checkpoint module (elastic#19560)
  Stop storing stateless kubernetes keystores (elastic#21880)
  [Elastic Agent] Fix named pipe communication on Windows 7 (elastic#21931)
  [Elastic Agent] Fix index for Agent monitoring to to elastic_agent. (elastic#21932)
@jsoriano jsoriano added v7.9.4 and removed v7.9.3 labels Jan 27, 2021
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Filebeat] Panic in K8s autodiscover
3 participants